Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore!: refactored the getTransactionCost method #2643

Merged
merged 40 commits into from
Jul 23, 2024
Merged

chore!: refactored the getTransactionCost method #2643

merged 40 commits into from
Jul 23, 2024

Conversation

petertonysmith94
Copy link
Contributor

@petertonysmith94 petertonysmith94 commented Jun 28, 2024 •
edited by arboleya
Loading

Release notes

In this release, we:

  • Refactored getTransactionCost method for Provider and Account

Summary

  • Refactored functionality for Provider.getTransactionCost to Account.getTransactionCost:
    • Separate concerns of estimating and fake funding a transaction.
    • Reducing unnecessary parameters, leading to a cleaner API.
  • Deprecated the Provider.getResourcesForTransaction method:
    • Lack of use-cases and deemed not required.

Breaking Changes

Refactored functionality for Provider.getTransactionCost to Account.getTransactionCost and changed estimation parameter from quantitiesToContract to quantities.

// before
const provider = Provider.create(...);
const account = Wallet.generate({ ... }) || new Predicate(...);
const quantities: Array<CoinQuantityLike> = [
  { amount: 1000, assetId: provider.getBaseAssetId() }
];

const cost = provider.getTransactionCost(txRequest, {
  resourceOwner: account,
  quantitiesToContract: quantities,
})
// after
const provider = Provider.create(...);
const account = Wallet.generate({ ... }) || new Predicate(...);
const quantities: Array<CoinQuantityLike> = [
  { amount: 1000, assetId: provider.getBaseAssetId() }
];

const cost = account.getTransactionCost(txRequest, { quantities });

Checklist

  • I added — tests to prove my changes
  • I updated — all the necessary docs
  • I reviewed — the entire PR myself, using the GitHub UI
  • I described — all breaking changes and the Migration Guide

@petertonysmith94 petertonysmith94 added the chore Issue is a chore label Jun 28, 2024
@petertonysmith94 petertonysmith94 added this to the 0.x mainnet milestone Jun 28, 2024
@petertonysmith94 petertonysmith94 self-assigned this Jun 28, 2024
Torres-ssf
Torres-ssf reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @petertonysmith94 I understand that this is a draft yet, just leave a couple of questions here to better understand your motivations

petertonysmith94
petertonysmith94 commented Jul 2, 2024
View reviewed changes
Copy link
Contributor Author

@petertonysmith94 petertonysmith94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Torres-ssf really appreciate the review on the draft :)

packages/account/src/account.ts Outdated Show resolved Hide resolved
packages/account/src/account.ts Outdated Show resolved Hide resolved
packages/program/src/functions/base-invocation-scope.ts Outdated Show resolved Hide resolved
@petertonysmith94 petertonysmith94 changed the title chore: refactored the getTransactionCost method chore!: refactored the getTransactionCost method Jul 8, 2024
@arboleya arboleya removed this from the 0.x mainnet milestone Jul 19, 2024
arboleya
arboleya reviewed Jul 21, 2024
View reviewed changes
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conflicts to fix.

Torres-ssf
Torres-ssf reviewed Jul 22, 2024
View reviewed changes
Copy link
Contributor

@Torres-ssf Torres-ssf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petertonysmith94 I feel like we need to update this doc page to say that the getTransactionCost method is provided by the Account class instead of the Provider class, what do you think?

packages/account/src/account.ts Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

Coverage Report:

Lines Branches Functions Statements
79.4%(+0.04%) 71.53%(-0.12%) 77.32%(+0.02%) 79.53%(+0.04%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/account/src/account.ts 82.31%
(+1.16%)		 65.07%
(+1.14%)		 82.35%
(+0.54%)		 82%
(+1.15%)
🔴 packages/account/src/providers/provider.ts 68.02%
(-0.82%)		 62.63%
(-1.19%)		 73.49%
(-0.31%)		 67.98%
(-0.81%)
🔴 packages/account/src/test-utils/launchNode.ts 98.27%
(+0.01%)		 85.24%
(+0.5%)		 100%
(+0%)		 98.36%
(+0.02%)
🔴 packages/contract/src/contract-factory.ts 58.33%
(-0.29%)		 46.42%
(+0.27%)		 54.54%
(+0%)		 58.33%
(-0.29%)
🔴 packages/contract/src/util.ts 93.75%
(+0.42%)		 0%
(+0%)		 66.66%
(+0%)		 88.88%
(+0.65%)
🔴 packages/create-fuels/src/cli.ts 85.5%
(+0.21%)		 37.5%
(-2.5%)		 100%
(+0%)		 85.5%
(+0.21%)

maschad
maschad approved these changes Jul 22, 2024
View reviewed changes
Copy link
Member

@maschad maschad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job!

@petertonysmith94 petertonysmith94 merged commit d4c4e55 into master Jul 23, 2024
25 checks passed
@petertonysmith94 petertonysmith94 deleted the ps/chore/refactor-get-transaction-cost branch July 23, 2024 06:06
@fuel-service-user fuel-service-user mentioned this pull request Jul 23, 2024
Merged
@Torres-ssf Torres-ssf mentioned this pull request Aug 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Torres-ssf Torres-ssf Torres-ssf approved these changes

@maschad maschad maschad approved these changes

@arboleya arboleya arboleya approved these changes

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

@Dhaiwat10 Dhaiwat10 Awaiting requested review from Dhaiwat10 Dhaiwat10 is a code owner

@danielbate danielbate Awaiting requested review from danielbate danielbate is a code owner

@nedsalk nedsalk Awaiting requested review from nedsalk nedsalk is a code owner

Assignees

@petertonysmith94 petertonysmith94

Labels
chore Issue is a chore
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Proposal to Simplify Provider.getTransactionCost
4 participants
@petertonysmith94 @Torres-ssf @arboleya @maschad